[#1246] Migrate referral POST endpoints to SIWE auth#1284
Conversation
Replace verifyWalletOwnership with verifySiweRequest on both register-referral POST and referral-code POST. Closes the activation-signature-replay attack surface (R2/R22) — SIWE validates domain, URI, chainId, statement, and 10-min freshness. GET endpoints remain unauthenticated (referral-code GET preserved for ShareButtons compatibility). Zero remaining verifyWalletOwnership calls in mutation endpoints. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
realproject7
left a comment
realproject7
left a comment
There was a problem hiding this comment.
@re2 review — APPROVE ✅
Checked against issue #1246 acceptance criteria:
| Criterion | Status |
|---|---|
| Both POST endpoints reject expired (>10 min) sigs → 401 | ✅ via verifySiweRequest |
| Both POST endpoints reject wrong domain/URI/chainId → 401 | ✅ |
| Valid POST → existing behavior unchanged | ✅ only auth path changed |
GET /api/airdrop/referral-code remains unauthenticated |
✅ GET handlers untouched |
No remaining verifyWalletOwnership in v5 mutations |
✅ (grep verified per PR) |
| Version 1.33.0 → 1.33.1 (patch) | ✅ |
| Lockfile synced | ✅ |
Code review notes:
- Surgical 2-file change: import swap + auth pattern update, nothing else touched
- Signature type relaxed from
`0x${string}`tostring— correct forverifySiweRequestAPI auth.errornow returned in 401 response — provides typed error info to clients (domain_mismatch, expired, etc.)- Rate limiting preserved on both endpoints
- Closes R2/R22 replay attack surface within SIWE freshness window
Clean security fix. No issues found.
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The code change appears to move both referral POST handlers to verifySiweRequest while leaving GET unauthenticated, which is the intended direction. However, this PR is a critical security migration and it adds no endpoint-level tests for the #1246 acceptance criteria.
Findings
- [high] The PR does not add route tests proving the two mutation endpoints now enforce SIWE semantics and preserve existing behavior. #1246 explicitly requires both POST endpoints to reject expired replay attempts and wrong domain/URI/chainId signatures with 401, valid signatures to keep existing behavior unchanged, and
GET /api/airdrop/referral-codeto remain unauthenticated with the existing{ code, is_farcaster_username? }response shape. Existinglib/airdrop/siwe-verify.test.tscovers the helper, but it does not prove these two endpoints are wired to it correctly or that failed auth avoids referral/code writes.- File:
src/app/api/airdrop/register-referral/route.ts:79 - File:
src/app/api/airdrop/referral-code/route.ts:63 - Suggestion: Add focused route tests for both POST handlers that mock
verifySiweRequestreturningexpired,domain_mismatch,uri_mismatch, andchain_id_mismatchand assert 401/no DB mutation; add valid-SIWE tests for the existing success paths; and add a GETreferral-codetest confirming no auth is required and the response shape is unchanged.
- File:
Decision
Requesting changes until the security acceptance criteria are covered at the endpoint boundary. CI was still pending at review time.
8 tests: expired/wrong-domain replay → 401 with no DB mutation on
both POST endpoints, valid SIWE success paths, GET referral-code
remains unauthenticated with { code, is_farcaster_username } shape.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The requested endpoint-level coverage has been added. Both referral mutation endpoints now use verifySiweRequest, auth failures return 401 without inserts, valid SIWE preserves the success flows, and referral-code GET remains unauthenticated with the expected response shape.
Findings
- None blocking.
Decision
Approving PR #1284. The security migration is scoped to the intended POST handlers, GET behavior is preserved, and the new route tests cover the reviewer-requested boundary behavior. CI was still pending at review time.
Replace 'as any' casts with Object.assign for NextRequest mock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use NextRequest constructor instead of Object.assign for type compat. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Re-reviewed the latest head after the test-only lint/type fixes. The referral POST SIWE migration and endpoint-level coverage remain intact, and the latest changes only replace the GET test request mock with NextRequest.
Findings
- None blocking.
Decision
Approving PR #1284 on the latest head. CI was still pending at review time; separately, Head has already requested the PR body metadata update to include Closes #1246 before merge.
2 participants
Summary
register-referralPOST andreferral-codePOST fromverifyWalletOwnership(signature-recovery only) toverifySiweRequest(full EIP-4361: domain, URI, chainId, statement, 10-min freshness)/api/airdrop/referral-codepreserved unauthenticated — ShareButtons/useReferralCode compatibilityverifyWalletOwnershipcalls in mutation endpoints (grep verified)Security
Intercepted activation
{message, signature}pairs can no longer be replayed against referral mutation endpoints after the 10-min SIWE freshness window.Version
1.33.0 → 1.33.1
🤖 Generated with Claude Code
Closes #1246